human-readable: calculate numbers in a loop#872
Conversation
989ba05 to
dd918b8
Compare
|
Ismail |
3e7e591 to
37afb8b
Compare
4da2ea0 to
a5defbf
Compare
b7ae0ad to
be26f4f
Compare
steadytao
left a comment
There was a problem hiding this comment.
This needs one portability fix before merge. The new loop calls labs(num / mult) but num is int64; on platforms where int64 is not long that can truncate or otherwise compare the wrong value for large inputs. The comparison can avoid that by staying in int64 arithmetic, for example by comparing both positive and negative ranges directly rather than calling labs().
The docs also list Z and Y but with the current int64 input those units are not reachable in practice. If the implementation only reaches E, I would keep the documentation to the reachable units.
|
Updated - here, and in #459 with some additional changes. |
steadytao
left a comment
There was a problem hiding this comment.
This is much closer but I think there is still one signed-range issue before merge.
pos_num = num * (num < 0 ? -1 : 1) overflows for INT64_MIN since the minimum signed value cannot be represented as a positive int64. The old code avoided that because it converted to double before negating.
Could the scale-selection loop avoid forming an absolute signed value? For example, compare num / mult against both positive and negative powi thresholds directly, or otherwise handle the minimum signed value without negating it.
|
Ah, yes... Unlikely to hit this 😁️ - but you are right. But it should be fine when using an unsinged data type? Switched to I would like to keep the variable, as a follow-up commit (currently in 61802ac) re-uses it. |
steadytao
left a comment
There was a problem hiding this comment.
Agreed that this is unlikely to be hit in normal use but this helper is specifically in the numeric formatting path and the fix is small enough that I think we should keep it defined for the full int64 range.
There is still one signed-range issue here: assigning to uint64_t does not avoid the overflow because num * -1 is evaluated in signed int64 before conversion. For INT64_MIN, that is still undefined behaviour.
This needs to convert before negating or avoid forming a signed absolute value. For example: uint64_t abs_num = num < 0 ? 0 - (uint64_t)num : (uint64_t)num;
That keeps the INT64_MIN case defined in unsigned arithmetic.
|
I did not want to say that the case is irrelevant, we should definitely get this right. I just wanted to explain why my mind did not came up with this. 😉️ Same for the overflow in calculation - I was not aware of this. Updated again, I hope it is ok now. Thanks a lot for the detailed review! |
steadytao
left a comment
There was a problem hiding this comment.
Thank you very much, this looks good to me now. Nice work on cleaning this up and sticking with my tendency for correctness 🤣
|
@tridge Could you review and merge when available, cheers mate! |
|
@eworm-de I will also have a more thorough look at 459 within the next 24-48 hours, thank you for your work thus far. |
|
Thanks a lot, and you are welcome! Let me know if you want #459 to be split into smaller pull requests for review. |
It's small enough, that'll be fine. |
6386859 to
b27da65
Compare
This drops a lot of `else if` blocks and extends units by "E" (Exa), which is 2^60 and thus the last to fit into int64.
This drops a lot of
else ifblocks and extends units by "E", "Z" & "Y".